Implement negation #30
Conversation
There was a problem hiding this comment.
@ip1981 Thank you so much for fixing this!
It's been on my list for so long, but I haven't had the brain space to learn how lalrpop works.
I've added a couple of comments below: the biggest thing to fix is the precedence ordering, and naming.
I think you'll likely end up making an UnOp60 (or similarly named rule).
Once you get operator precedence squared away, re-request a review from me!
Many thanks, again!
jexl-parser/src/parser.lalrpop
Outdated
| @@ -116,6 +117,10 @@ Op50: OpCode = { | |||
| "^" => OpCode::Exponent, | |||
| }; | |||
|
|
|||
| Op60: UnOpCode = { | |||
There was a problem hiding this comment.
Nit: given this is at Expr70 level, to follow the rest of the code, could we name this Op80?
jexl-parser/src/parser.lalrpop
Outdated
| @@ -67,6 +67,7 @@ Expr60: Box<Expression> = { | |||
| Expr70: Box<Expression> = { | |||
| <subject: Expr70> <index: Index> => Box::new(Expression::IndexOperation{subject, index}), | |||
| <subject: Expr70> "." <ident: Identifier> => Box::new(Expression::DotOperation{subject, ident}), | |||
| <operation: Op60> <right: Expr80> => Box::new(Expression::UnaryOperation { operation, right }), | |||
There was a problem hiding this comment.
Should . and [ bind tighter than !? Playground with the jexl-js.
I couldn't see a test, and I'm not familiar enough with lalrpop know. Please could you add tests verifying the following work:
foo == [false]
!foo[0] == true
foo == { "bar": false }
!foo.bar == true
There was a problem hiding this comment.
Ok, I've updated my patch fixing priorities, naming, adding the tests you suggested.
|
I would be great if this PR could be merge to implement the |
...and also it seems it would help to make the unary minus to work (issue #27)! |
Signed-off-by: Igor Pashev <[email protected]>
No description provided.